Conversation
…cuted The HookExecutor was missing invocation of BeforeEvery(Class), AfterEvery(Class), BeforeEvery(Assembly), and AfterEvery(Assembly) hooks. While the Test-level hooks had the correct pattern of executing both BeforeEvery and Before hooks, the same pattern was missing for Class and Assembly levels. Updated ExecuteBeforeClassHooksAsync, ExecuteAfterClassHooksAsync, ExecuteBeforeAssemblyHooksAsync, and ExecuteAfterAssemblyHooksAsync to also collect and execute the corresponding BeforeEvery/AfterEvery hooks, following the same pattern already used for Test-level hooks. Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/89836764-b81e-472f-9f00-c6e7e61c02ca
|
@copilot we need tests to ensure this never regresses |
Test targets in TUnit.TestProject: - BeforeEveryClassTests: [BeforeEvery(Class)] hook sets StateBag flag, test verifies - BeforeEveryAssemblyTests: [BeforeEvery(Assembly)] hook sets StateBag flag, test verifies - AfterEveryClassTests: [AfterEvery(Class)] hook writes file as proof of execution - AfterEveryAssemblyTests: [AfterEvery(Assembly)] hook writes file as proof of execution Integration test runners in TUnit.Engine.Tests: - BeforeEveryClassHookTests: Runs filtered tests, asserts 1 passed / 0 failed - BeforeEveryAssemblyHookTests: Same pattern for assembly-level hooks - AfterEveryClassHookTests: Validates test passes and hook file was created - AfterEveryAssemblyHookTests: Validates test passes and hook file was created Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/3e2fe83b-5e07-41db-9d58-975349f40a31
Added 8 regression test files in TUnit.TestProject (test targets):
TUnit.Engine.Tests (integration test runners):
These follow the same patterns as the existing |
There was a problem hiding this comment.
Review: Fix BeforeEvery/AfterEvery hooks for Class and Assembly
The fix is correct and follows the existing pattern established for ExecuteBeforeTestHooksAsync / ExecuteAfterTestHooksAsync. The root cause was clearly identified — missing CollectBeforeEvery* / CollectAfterEvery* call sites — and the regression tests adequately cover the previously broken paths.
Primary Concern: Code Duplication in HookExecutor.cs
The main architectural issue is that this PR copies the same 12-line try/catch block four times across ExecuteBeforeClassHooksAsync and ExecuteBeforeAssemblyHooksAsync:
catch (Exception ex)
{
if (ex is SkipTestException) { throw; }
if (ex.InnerException is SkipTestException skipEx)
{
ExceptionDispatchInfo.Capture(skipEx).Throw();
}
throw new BeforeClassException($"... hook failed: {ex.Message}", ex);
}This pattern already existed once in the BeforeTest variant and is now duplicated three more times. The concern is maintainability: any future change to this pattern (e.g., adding a new exception type, changing wrapping behaviour) must be made in all four places. A private helper method would consolidate this:
private static async ValueTask ExecuteBeforeHookWithSkipHandling<TException>(
Func<ValueTask> execute,
Func<string, Exception, TException> wrapException)
where TException : Exception
{
try { await execute().ConfigureAwait(false); }
catch (Exception ex)
{
if (ex is SkipTestException) throw;
if (ex.InnerException is SkipTestException skipEx)
ExceptionDispatchInfo.Capture(skipEx).Throw();
throw wrapException($"hook failed: {ex.Message}", ex);
}
}This isn't blocking — the current duplication was already present for the Test variant before this PR — but it's worth addressing either here or as a follow-up to prevent the pattern from spreading further.
Minor Observations
Test isolation for BeforeEveryAssembly: The BeforeEveryAssemblyHooks iterates context.AllTests (all tests in the assembly) and filters by exact test name. This works today but is brittle if the test is renamed without updating the hook. A better design in tests is to scope the filter by class name or use a dedicated attribute/tag rather than string-matching test names.
AfterEvery file-system side-channel: Using File.WriteAllTextAsync with a GUID filename is an acceptable trade-off for verifying AfterEvery execution (since the hook runs after the test's assertion window closes). It would be worth noting that these files are never cleaned up — if this is a concern in CI environments, a cleanup step may be needed.
Removal of the empty-collection early return in ExecuteAfterClassHooksAsync / ExecuteAfterAssemblyHooksAsync: The original code returned early with FinishClassActivity(hasErrors: false) when the hooks collection was empty. The new code doesn't have this early return, but FinishClassActivity / FinishAssemblyActivity is still called at the bottom unconditionally — so this is fine and actually cleaner.
Summary
The bug fix is correct and well-tested. The main suggestion is to extract the repeated SkipTestException-handling try/catch into a helper to prevent further duplication, ideally as a follow-up since this pattern predates the PR.
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.21.6 to 1.23.7. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.23.7 <!-- Release notes generated using configuration in .github/release.yml at v1.23.7 --> ## What's Changed ### Other Changes * feat: use results directory provided by Microsoft Testing Platform in HtmlReporter by @DavidZidar in thomhurst/TUnit#5294 * feat: add benchmarks for Imposter and Mockolate mocking frameworks by @vbreuss in thomhurst/TUnit#5295 * feat: add TUnit0080 analyzer for missing polyfill types by @thomhurst in thomhurst/TUnit#5292 * fix: respect user-set TUnitImplicitUsings from Directory.Build.props by @thomhurst in thomhurst/TUnit#5280 * perf: optimize TUnit.Mocks hot paths by @thomhurst in thomhurst/TUnit#5300 ### Dependencies * chore(deps): update tunit to 1.22.19 by @thomhurst in thomhurst/TUnit#5296 ## New Contributors * @DavidZidar made their first contribution in thomhurst/TUnit#5294 **Full Changelog**: thomhurst/TUnit@v1.22.19...v1.23.7 ## 1.22.19 <!-- Release notes generated using configuration in .github/release.yml at v1.22.19 --> ## What's Changed ### Other Changes * Add mock library benchmarks: TUnit.Mocks vs Moq, NSubstitute, FakeItEasy by @Copilot in thomhurst/TUnit#5284 * perf: lazily initialize optional MockEngine collections by @thomhurst in thomhurst/TUnit#5289 * Always emit TUnit.Mocks.Generated namespace from source generator by @Copilot in thomhurst/TUnit#5282 ### Dependencies * chore(deps): update tunit to 1.22.6 by @thomhurst in thomhurst/TUnit#5285 **Full Changelog**: thomhurst/TUnit@v1.22.6...v1.22.19 ## 1.22.6 <!-- Release notes generated using configuration in .github/release.yml at v1.22.6 --> ## What's Changed ### Other Changes * fix: use IComputeResource to filter waitable Aspire resources by @thomhurst in thomhurst/TUnit#5278 * fix: preserve StateBag when creating per-test TestBuilderContext by @thomhurst in thomhurst/TUnit#5279 ### Dependencies * chore(deps): update tunit to 1.22.3 by @thomhurst in thomhurst/TUnit#5275 **Full Changelog**: thomhurst/TUnit@v1.22.3...v1.22.6 ## 1.22.3 <!-- Release notes generated using configuration in .github/release.yml at v1.22.3 --> ## What's Changed ### Other Changes * fix: pass assembly version properties to dotnet pack by @thomhurst in thomhurst/TUnit#5274 ### Dependencies * chore(deps): update tunit to 1.22.0 by @thomhurst in thomhurst/TUnit#5272 **Full Changelog**: thomhurst/TUnit@v1.22.0...v1.22.3 ## 1.22.0 <!-- Release notes generated using configuration in .github/release.yml at v1.22.0 --> ## What's Changed ### Other Changes * perf: run GitVersion once in CI instead of per-project by @slang25 in thomhurst/TUnit#5259 * perf: disable GitVersion MSBuild task globally by @thomhurst in thomhurst/TUnit#5266 * fix: skip IResourceWithoutLifetime resources in Aspire fixture wait logic by @thomhurst in thomhurst/TUnit#5268 * fix: relax docs site Node.js engine constraint to >=24 by @thomhurst in thomhurst/TUnit#5269 * fix: catch unhandled exceptions in ExecuteRequestAsync to prevent IDE RPC crashes by @thomhurst in thomhurst/TUnit#5271 * feat: register HTML report as MTP session artifact by @thomhurst in thomhurst/TUnit#5270 ### Dependencies * chore(deps): update tunit to 1.21.30 by @thomhurst in thomhurst/TUnit#5254 * chore(deps): update opentelemetry to 1.15.1 by @thomhurst in thomhurst/TUnit#5258 * chore(deps): bump node-forge from 1.3.1 to 1.4.0 in /docs by @dependabot[bot] in thomhurst/TUnit#5255 * chore(deps): bump picomatch from 2.3.1 to 2.3.2 in /docs by @dependabot[bot] in thomhurst/TUnit#5256 * chore(deps): update react by @thomhurst in thomhurst/TUnit#5261 * chore(deps): update node.js to >=18.20.8 by @thomhurst in thomhurst/TUnit#5262 * chore(deps): update node.js to v24 by @thomhurst in thomhurst/TUnit#5264 **Full Changelog**: thomhurst/TUnit@v1.21.30...v1.22.0 ## 1.21.30 <!-- Release notes generated using configuration in .github/release.yml at v1.21.30 --> ## What's Changed ### Other Changes * feat: add test discovery Activity span for tracing by @thomhurst in thomhurst/TUnit#5246 * Fix mock generator not preserving nullable annotations on reference types by @Copilot in thomhurst/TUnit#5251 * Fix ITestSkippedEventReceiver not firing for [Skip]-attributed tests by @thomhurst in thomhurst/TUnit#5253 * Use CallerArgumentExpression for TestDataRow by default. by @m-gasser in thomhurst/TUnit#5135 ### Dependencies * chore(deps): update tunit to 1.21.24 by @thomhurst in thomhurst/TUnit#5247 **Full Changelog**: thomhurst/TUnit@v1.21.24...v1.21.30 ## 1.21.24 <!-- Release notes generated using configuration in .github/release.yml at v1.21.24 --> ## What's Changed ### Other Changes * Fix OpenTelemetry missing root span by reordering session activity lifecycle by @Copilot in thomhurst/TUnit#5245 ### Dependencies * chore(deps): update tunit to 1.21.20 by @thomhurst in thomhurst/TUnit#5241 * chore(deps): update dependency stackexchange.redis to 2.12.8 by @thomhurst in thomhurst/TUnit#5243 **Full Changelog**: thomhurst/TUnit@v1.21.20...v1.21.24 ## 1.21.20 <!-- Release notes generated using configuration in .github/release.yml at v1.21.20 --> ## What's Changed ### Other Changes * fix: respect TUnitImplicitUsings set in Directory.Build.props by @thomhurst in thomhurst/TUnit#5225 * feat: covariant assertions for interfaces and non-sealed classes by @thomhurst in thomhurst/TUnit#5226 * feat: support string-to-parseable type conversions in [Arguments] by @thomhurst in thomhurst/TUnit#5227 * feat: add string length range assertions by @thomhurst in thomhurst/TUnit#4935 * Fix BeforeEvery/AfterEvery hooks for Class and Assembly not being executed by @Copilot in thomhurst/TUnit#5239 ### Dependencies * chore(deps): update tunit to 1.21.6 by @thomhurst in thomhurst/TUnit#5228 * chore(deps): update dependency gitversion.msbuild to 6.7.0 by @thomhurst in thomhurst/TUnit#5229 * chore(deps): update dependency gitversion.tool to v6.7.0 by @thomhurst in thomhurst/TUnit#5230 * chore(deps): update aspire to 13.2.0 - autoclosed by @thomhurst in thomhurst/TUnit#5232 * chore(deps): update dependency typescript to v6 by @thomhurst in thomhurst/TUnit#5233 * chore(deps): update dependency polyfill to 9.23.0 by @thomhurst in thomhurst/TUnit#5235 * chore(deps): update dependency polyfill to 9.23.0 by @thomhurst in thomhurst/TUnit#5236 **Full Changelog**: thomhurst/TUnit@v1.21.6...v1.21.20 Commits viewable in [compare view](thomhurst/TUnit@v1.21.6...v1.23.7). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
[BeforeEvery(Class)],[BeforeEvery(Assembly)],[AfterEvery(Class)], and[AfterEvery(Assembly)]hooks were silently never invoked, while theTestandTestSessionvariants worked fine.Root cause
HookExecutoralready had the correct pattern for Test-level hooks — collecting and executing bothBeforeEvery(Test)andBefore(Test)hooks in sequence. The Class and Assembly methods only calledCollectBefore{Class,Assembly}HooksAsyncand never called the correspondingCollectBeforeEvery*/CollectAfterEvery*methods. The hooks were registered and built correctly in both the source generator and reflection paths; only the execution callsite was missing.Changes
ExecuteBeforeClassHooksAsync/ExecuteBeforeAssemblyHooksAsync: now executeBeforeEvery(*)hooks beforeBefore(*)hooksExecuteAfterClassHooksAsync/ExecuteAfterAssemblyHooksAsync: now executeAfter(*)hooks beforeAfterEvery(*)hooks (cleanup in reverse order)Follows the identical pattern already established by
ExecuteBeforeTestHooksAsync/ExecuteAfterTestHooksAsync.Regression Tests
Added integration tests to ensure all four hook types are properly executed:
TUnit.TestProject/BeforeTests/BeforeEveryClassTests.cs:[BeforeEvery(Class)]hook sets aStateBagflag; test verifies it was setTUnit.TestProject/BeforeTests/BeforeEveryAssemblyTests.cs:[BeforeEvery(Assembly)]hook sets aStateBagflag; test verifies it was setTUnit.TestProject/AfterTests/AfterEveryClassTests.cs:[AfterEvery(Class)]hook writes a file as proof of executionTUnit.TestProject/AfterTests/AfterEveryAssemblyTests.cs:[AfterEvery(Assembly)]hook writes a file as proof of executionTUnit.Engine.Tests/BeforeEveryClassHookTests.cs/BeforeEveryAssemblyHookTests.cs: integration test runners that execute filtered tests and assert 1 passed / 0 failedTUnit.Engine.Tests/AfterEveryClassHookTests.cs/AfterEveryAssemblyHookTests.cs: integration test runners that validate tests pass and hook files were createdOriginal prompt
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.